Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix data-turbo-confirm on <a> without data-turbo-method #874

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix data-turbo-confirm on <a> without data-turbo-method #874

wants to merge 1 commit into from

Conversation

marcoroth
Copy link
Member

@marcoroth marcoroth commented Feb 16, 2023 •
edited
Loading

The documentation in the "Requiring Confirmation for a Visit" section mentions the use of the [data-turbo-confirm] attribute on an <a> element to prompt a confirmation before executing a visit. An example is given as follows:

<a href="/articles" data-turbo-confirm="Do you want to leave this page?">Back to articles</a>

Prior to version v7.2.5, this functionality did not work unless the [data-turbo-method] attribute was also present on the link.

This pull request modifies the behavior so that [data-turbo-confirm] can now be used on links without the [data-turbo-method] attribute.

Resolves #943
Resolves #1264
Resolves #1296
Closes #1266

seanpdoyle
seanpdoyle suggested changes Feb 16, 2023
View reviewed changes
Copy link
Contributor

@seanpdoyle seanpdoyle left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the tests and the underlying behavior here.

Having said that, I worry that the FormLinkClickObserver is an inappropriate candidate for supporting that behavior.

Would an <a> without [data-turbo-method] be treated as a typical Visit navigation?

In the absence of that attribute, what is directing the code to flow through this observer? Could confirmation be accommodated elsewhere, closer to a typical <a>-driven Visit?

@marcoroth marcoroth force-pushed the data-turbo-confirm-without-data-turbo-method-on-anchors branch from 634dd95 to 2bf844d Compare February 16, 2023 21:24
@marcoroth
Copy link
Member Author

marcoroth commented Feb 16, 2023 •
edited
Loading

Thanks @seanpdoyle, this is good feedback! You are right, this logic doesn't belong to the FormLinkClickObserver.

I pushed up a version where we check for the presence of the data-turbo-confirm attribute in the regular visit flow for links in Session.

Since confirmMethod was tied to FormSubmission I extracted it into a new Confirmation class which both the form flow and link flow now make use of. I like this a lot better!

@marcoroth marcoroth force-pushed the data-turbo-confirm-without-data-turbo-method-on-anchors branch from 2bf844d to b9bc188 Compare February 16, 2023 21:45
@tkoenig tkoenig mentioned this pull request Jul 11, 2023
Open
@searls
Copy link

searls commented Jan 5, 2024

Seems weird that the documentation still says this is possible when the example cited doesn't actually work:

<a href="/articles" data-turbo-confirm="Do you want to leave this page?">Back to articles</a>

Just spent 15 minutes verifying it wasn't a problem in my turbo version or import maps setup

@marcoroth
Copy link
Member Author

Let me get this rebased so we get a chance to get this this merged in

@marcoroth marcoroth force-pushed the data-turbo-confirm-without-data-turbo-method-on-anchors branch from b9bc188 to c8f05e9 Compare May 29, 2024 16:35
@marcoroth marcoroth mentioned this pull request May 29, 2024
Open
@marcoroth marcoroth force-pushed the data-turbo-confirm-without-data-turbo-method-on-anchors branch from c8f05e9 to f3682bd Compare May 29, 2024 16:36
searls added a commit to searls/turbo-site that referenced this pull request Jul 5, 2024
@searls
Fix inaccurate doc
9fd6bb6 
Until hotwired/turbo#874 or hotwired/turbo#1266 resolve hotwired/turbo#1264 and  hotwired/turbo#943, the doc site is wrong and I keep wasting time double-checking it, only for its wrongness to result in me wasting more time debugging Turbo's source code.
@searls searls mentioned this pull request Jul 5, 2024
Open
searls added a commit to searls/turbo-site that referenced this pull request Jul 5, 2024
@searls
Fix inaccurate doc
e2a3fe9 
Until hotwired/turbo#874 or hotwired/turbo#1266 resolve hotwired/turbo#1264 and  hotwired/turbo#943, the doc site is wrong and I keep wasting time double-checking it, only for its wrongness to result in me wasting more time debugging Turbo's source code.
@marcoroth marcoroth force-pushed the data-turbo-confirm-without-data-turbo-method-on-anchors branch from f3682bd to 4c3c658 Compare August 16, 2024 16:46
@marcoroth
Copy link
Member Author

I just rebased this PR again, but I feel like this is not quite right anymore after the merged PR #1217 from @seanpdoyle. The new Turbo.config.forms.confirm config option doesn't quite fit the naming now, since the confirm method is also used on non-form elements here.

I'm wondering if we should either rename the config option or also use the Turbo.config.forms.confirm config option for the <a> tags.

@marcoroth marcoroth mentioned this pull request Aug 18, 2024
Open
@marcoroth marcoroth force-pushed the data-turbo-confirm-without-data-turbo-method-on-anchors branch from 4c3c658 to bbd65cc Compare August 18, 2024 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seanpdoyle seanpdoyle seanpdoyle requested changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
3 participants
@marcoroth @searls @seanpdoyle